Skip to content

fix(security): make transactions server-write-only so a pending row cannot under-quote what it owes (#538) - #539

Merged
guillermoscript merged 1 commit into
masterfrom
fix/transaction-insert-grant-538
Jul 25, 2026
Merged

fix(security): make transactions server-write-only so a pending row cannot under-quote what it owes (#538)#539
guillermoscript merged 1 commit into
masterfrom
fix/transaction-insert-grant-538

Conversation

@guillermoscript

Copy link
Copy Markdown
Owner

What

transactions becomes server-write-only: authenticated loses its INSERT grant, and the two API routes that used it (/api/payments/checkout, /api/stripe/create-payment-intent) write on the admin client instead. A student can no longer open a pending row that under-quotes what it owes.

Closes #538

Why

#528 closed self-declared payment by pinning status = 'pending' on the user-scoped INSERT policy. It deliberately said nothing about what a legitimately pending row claims it is owed — and the Solana settlement columns are exactly that. Both Solana paths verify against the row's own figures:

  • lib/payments/solana-reconcile.ts:104verifySplitTransfer({ totalBase: settlement_base }), reached from /api/payments/solana/verify and the /api/cron/solana-reconcile backstop
  • app/api/payments/solana/tx/route.ts:119 → the payment request the wallet is handed

So a student could POST /rest/v1/transactions directly with a pending solana row for a $49 product carrying settlement_currency: 'sol', settlement_base: 1, pay one lamport, and have the verify endpoint confirm the transfer and flip the row to successful on the service-role client — firing trigger_manage_transactions and granting the entitlement. Full course access for a fraction of a cent, through the legitimate payment path. Reproduced against a local database:

 transaction_id | amount | settlement_currency | settlement_base
----------------+--------+---------------------+-----------------
          10004 | 100.00 | sol                 |               1

The same row shape also under-quotes amount. That does not buy access for Lemon Squeezy / PayPal / Binance — those charge from their own catalogue, and lib/payments/webhook-dispatch.ts never reads amount — but getPayoutsOwed() sums it, so an under-quoted row understates what the platform owes the school.

Which columns are load-bearing (the issue asks this before choosing a control):

Column Load-bearing?
settlement_base Yes — the figure compared on-chain, and the amount the QR asks for
settlement_currency Yes — sets decimals (6 for usdc, else 9); wrong decimals move the verified amount by 10³
settlement_mint Yes — selects which SPL token counts; a caller-chosen mint is one the attacker can mint freely
settlement_sol_usd No — nothing reads it. Audit record of the locked quote only
amount Yes, twice over — the fallback both Solana paths use when settlement_base IS NULL, and the payout figure

The root cause is the grant, not the columns. Nothing in the browser inserts into transactions; every legitimate insert is already server-side, and the two API routes are the only reason authenticated held the grant. Both already derive amount, currency and payment_provider from tenant-scoped products / plans reads, and the settlement figures from getSolUsdPrice(). So:

  1. Both inserts move to the admin client — the pattern transactions RLS restricts rows but not columns — a student can self-insert a 'successful' sale #528 applied to enrollUser. user_id comes from the verified session and tenant_id from the x-tenant-id header, so the tenant validation CLAUDE.md requires is already in place.
  2. REVOKE INSERT ON public.transactions FROM authenticated, anonPOST /rest/v1/transactions now fails with permission denied for table transactions. Combined with transactions RLS restricts rows but not columns — a student can self-insert a 'successful' sale #528's UPDATE column grant, amount and all four settlement columns are unreachable by an untrusted caller on both verbs.
  3. transactions RLS restricts rows but not columns — a student can self-insert a 'successful' sale #528's INSERT policy is kept and tightened to pin the four settlement columns NULL. It is unreachable while the grant is revoked; it is there so this specific under-quote stays impossible if the grant ever returns (a rollback, a schema dump re-applying the original GRANT ALL, or someone resolving a permission denied error the direct way).

Two directions from the issue were considered and rejected:

  • A BEFORE INSERT trigger recomputing the values (the Split snapshot is written at a single call site with no DB backstop (#496 follow-up) #512 pattern) would overwrite legitimate ones — completing a payment request inserts the admin-confirmed payment_amount, and grant_free_subscription() inserts a zero-amount row for its plan. It also cannot help with native SOL, whose settlement_base comes from a live quote a trigger cannot obtain.
  • A SECURITY DEFINER RPC has the same blind spot from the other side: it would have to accept the SOL/USD rate as a caller parameter, and that rate is precisely the value that must not be caller-supplied since settlement_base is derived from it. It moves the vulnerability rather than closing it. The admin-client insert keeps the quote from crossing the client boundary in either direction.

How to QA

On a fresh npm run db:reset (the migration is included in the chain), with npm run dev running:

  1. The vulnerability is closed. Run the verify snippet — every row should report PASS, including a live attempt at the exact insert from the issue:
    docker exec -i supabase_db_lms-front psql -U postgres -d postgres \
      -f - < supabase/snippets/verify_transactions_insert_lockdown.sql
  2. The regression spec. Covers both halves — a signed-in student cannot insert (under-quoted or honest), and /api/payments/checkout still creates the pending row priced from the product:
    npx playwright test transaction-insert-lockdown --workers=1
    To confirm these are real regression tests, apply supabase/migrations/rollback/20260725180000_transactions_insert_lockdown.down.sql and re-run -g "INSERT is closed" — both negative tests fail. Re-apply the migration afterwards (or npm run db:reset).
  3. The legitimate Solana path by hand, if Solana is configured for the school: buy a solana product as alice@student.com on code-academy.lvh.me:3005 and confirm the pending row still lands with the correct settlement_base / settlement_currency / settlement_mint, written by the admin client.
  4. The free-plan path (the one place a SECURITY DEFINER function inserts as a user): claim a free plan and confirm the zero-amount successful row is still created. Its DEFINER owner holds the INSERT privilege, so the revoke does not reach it.

Screenshots / GIF

Not applicable — no user-visible change. This is a database grant plus two server-side client swaps; the checkout UI is byte-identical.

Checklist

  • npm run typecheck and npm run test:unit pass (370 unit tests)
  • npm run build passes
  • Every new tenant-scoped query filters by tenant_id (or the table genuinely has no such column)
  • Tested with every relevant role (student / teacher / admin) — student is the only role with a path to this table; the admin-side manual-payment insert already used the admin client and is unchanged
  • Loading and error states handled — the routes' existing Failed to create transaction branch covers the insert
  • New UI strings added to both messages/en.json and messages/es.json — n/a, no UI strings
  • Migration applies cleanly on npm run db:reset, has RLS policies, and lib/database.types.ts was regenerated — no regeneration needed, the migration adds no columns or types

Test results

  • npx playwright test transaction-insert-lockdown --workers=116/16 pass across all four projects
  • parallel-subscription-guard, payment-flows, enrollment-flows, entitlements-overlap, tenant-isolationall pass on a fresh db:reset
  • Two failures showed up while testing and are pre-existing on master — verified by stashing this branch's changes, resetting the database, and re-running: auth-security.spec.ts:23 (sign-up form fields) and subscription-lapse.spec.ts:113 (perpetual product entitlement from seed data). Neither touches transaction grants. subscription-lapse also leaves alice without her seeded subscription, which is what makes parallel-subscription-guard fail if it runs afterwards in the same session.
  • npx eslint on the changed files — 0 errors, 1 pre-existing warning (itemName unused, create-payment-intent/route.ts:125, untouched by this PR)

Notes for the reviewer

…annot under-quote what it owes (#538)

#528 pinned `status = 'pending'` on the user-scoped INSERT policy, which closed
self-declared payment but said nothing about what a legitimately pending row
claims it OWES. The Solana settlement columns are exactly that, and both Solana
paths verify against them: `lib/payments/solana-reconcile.ts` builds
`verifySplitTransfer({ totalBase })` from `settlement_base`, and
`/api/payments/solana/tx` builds the wallet's payment request from it.

So a student could POST `/rest/v1/transactions` directly with a pending `solana`
row for a $49 product carrying `settlement_currency: 'sol', settlement_base: 1`,
pay one lamport, and have the verify endpoint confirm the transfer and flip the
row to 'successful' on the service-role client — firing
`trigger_manage_transactions` and granting the entitlement. Full course access
for a fraction of a cent, through the legitimate payment path. Reproduced
against a local database; the same row shape also under-quotes `amount`, which
understates the school's payout for the providers that charge from their own
catalogue.

Which columns are load-bearing, since the issue asks before choosing a control:
`settlement_base` (the figure compared on-chain), `settlement_currency` (sets
decimals, 6 vs 9), `settlement_mint` (which SPL token counts) and `amount` (the
legacy fallback, and the payout figure). `settlement_sol_usd` is read by
nothing — it is the audit record of the locked quote.

The root cause is the INSERT grant, not the columns: nothing in the browser
inserts into `transactions`, and both API routes that do already derive every
financial field server-side from tenant-scoped `products`/`plans` reads. So
their inserts move to the admin client — the pattern #528 applied to
`enrollUser` — and the grant is revoked from `authenticated` and `anon`. The
#528 policy is kept and tightened to pin the four settlement columns NULL, so
the under-quote stays impossible if the grant is ever restored.

Rejected: a BEFORE INSERT trigger recomputing `amount` (it would overwrite the
admin-confirmed `payment_amount` on manual payments and
`grant_free_subscription()`'s zero-amount row), and a SECURITY DEFINER RPC (it
cannot fetch the SOL/USD quote, so it would have to accept the rate from the
caller — the very value that must not be caller-supplied).

Verified against a local database: the under-quoted insert goes from succeeding
to `permission denied`; the same insert as service role still succeeds;
`grant_free_subscription()` still inserts as `authenticated` through its DEFINER
owner; and `/api/payments/checkout` still creates the pending row end-to-end.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P8j7YxZRVPT3B4VpENtpng
@guillermoscript guillermoscript added bug Something isn't working security Security vulnerability or hardening severity:high High severity security finding payments Payment provider / billing related labels Jul 25, 2026
@guillermoscript guillermoscript self-assigned this Jul 25, 2026
@guillermoscript
guillermoscript marked this pull request as ready for review July 25, 2026 20:20
@guillermoscript
guillermoscript merged commit 146a8cf into master Jul 25, 2026
2 checks passed
@guillermoscript
guillermoscript deleted the fix/transaction-insert-grant-538 branch July 25, 2026 21:37
guillermoscript added a commit that referenced this pull request Jul 26, 2026
…ke migration drift detectable (#541) (#552)

* fix(security): apply the transactions INSERT lockdown to cloud and make drift detectable (#541)

The #538 lockdown (20260725180000) was never applied to the cloud project. Until
this commit, `authenticated` still held INSERT on `public.transactions` there and
the INSERT policy was the older #528 shape, so a caller could open a pending row
quoting `settlement_base: 1` against a priced product, pay that on-chain, and
have /api/payments/solana/verify — which verifies against the row's own
settlement_base — flip it to successful and grant the entitlement.

Applied to cloud (verified by querying the live catalog, not by reading files):

  - REVOKE INSERT ON transactions FROM authenticated, anon
    INSERT is now held only by postgres and service_role.
  - INSERT policy re-created with all four settlement_* IS NULL pins.

Nothing depended on the grant: PR #539 had already moved both user-scoped
inserts onto createAdminClient(). The two user-scoped .update() calls that
remain in the checkout route only touch provider_subscription_id and status,
both inside the #528 three-column UPDATE grant, so they are unaffected.

WHY IT WAS LOST, AND WHAT NOW CATCHES IT

Cloud migration stamps had drifted from repo filenames: four migrations were
applied through the MCP apply_migration tool, which stamps a fresh timestamp
instead of the filename. That makes "what is missing from cloud?" unanswerable —
the re-stamped four read as pending, and the genuinely missing fifth was
indistinguishable from them.

  npm run verify:cloud

queries the live database and exits non-zero on drift. It asserts ledger
integrity (every repo migration stamped under its own filename; no stamp
matching no file) and the #512/#528/#538 payment invariants against catalog
state rather than migration text, so a later re-widening fails it too.

The rules are split into scripts/lib/verify-cloud-schema-checks.ts and driven
from both healthy and drifted fixtures in tests/unit/verify-cloud-schema.test.ts
(13 tests). The healthy fixture is the exact state read back from cloud, so the
policy-pin matcher is tested against how Postgres actually renders the
expression. That is also how the "fails if you re-grant INSERT" criterion is
demonstrated: as a repeatable test, rather than by briefly re-opening a write
grant on the payments table of the only production database this project has.

docs/MIGRATIONS.md now states the push-only rule and why, with the incident
table, and reframes the Management API fallback so the schema_migrations stamp
reads as the thing that makes it safe rather than optional bookkeeping.

Gates: typecheck clean, test:unit 383 passed (370 baseline + 13), eslint clean
on new files, build succeeds.

Refs #540

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HPFyHjqzSR6Ku1gyWjeQie

* fix(db): repair the drifted cloud migration ledger (#541)

Applied to cloud. Re-stamps the four migrations that had been applied through
the MCP apply_migration tool (which stamps a fresh timestamp instead of the
migration's filename) and records 20260725180000, whose DDL was applied earlier
in this issue:

  20260721120000_add_binance_personal_provider        was 20260725213441
  20260725110000_transaction_split_snapshot_backstop  was 20260725213508
  20260725160000_entitlement_gated_enrollment_inserts was 20260725213610
  20260725170000_transactions_column_hardening        was 20260725192946
  20260725180000_transactions_insert_lockdown         was absent

Each of the four was confirmed genuinely live by querying what its DDL created
before being treated as a ledger-only repair, so this re-runs no DDL.

The file is committed under the stamp apply_migration assigned it
(20260726005843) so the repair is not itself an orphan — the ledger now matches
the repo exactly: 171 files, 171 stamps, zero pending, zero orphans.

Idempotent: on a fresh `supabase db reset` the four UPDATEs match nothing and
the INSERT no-ops, since the CLI has already stamped 20260725180000 by the time
this file runs.

verify:cloud now reports 10/10 PASS.

Gates: typecheck clean, test:unit 383 passed.

Refs #540

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HPFyHjqzSR6Ku1gyWjeQie

* docs(migrations): note that verify:cloud's ledger checks are branch-relative (#541)

The two ledger checks compare cloud against the migrations in the CURRENT
checkout, so on a feature branch a migration another in-flight branch has
already applied reads as an orphan, and one on this branch not yet applied
reads as pending. Neither is drift.

Found while confirming #541: PRs #553 (#542) and #554 (#543) had applied
20260726013256, 20260726015858, 20260726100000 and 20260726110000 to cloud,
none of which exist on this branch — so verify:cloud run here reports four
orphans that are not drift at all.

Left as guidance rather than logic. Filtering by "is this stamp on some other
branch" would need a remote ref walk and would silently excuse the exact
condition the check exists to catch. The orphan detail line now names the
possibility and says to re-check on master; the payment-invariant checks are
branch-independent and meaningful anywhere.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HPFyHjqzSR6Ku1gyWjeQie

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working payments Payment provider / billing related security Security vulnerability or hardening severity:high High severity security finding

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Solana settlement columns are caller-controlled at INSERT — a pending row can under-quote what it owes (#528 follow-up)

1 participant